Skip to content

Stackoverflow fix in ImageDescriptor#3315

Merged
akurtakov merged 1 commit into
eclipse-platform:masterfrom
akurtakov:master
Sep 23, 2025
Merged

Stackoverflow fix in ImageDescriptor#3315
akurtakov merged 1 commit into
eclipse-platform:masterfrom
akurtakov:master

Conversation

@akurtakov
Copy link
Copy Markdown
Member

Fixes getImageData(100) calling itself endlessly (introduced via c2fe144 ) by reintroducing the bigger getImageData(100) and getImageData() endlessly (introduced in 2017 via
e6d12ca ) that wasn't experienced thanks to the happy accident that it was asbtract method and all implementations before that had their own and every implementor since then most likely figured the endless loop while developing.
Enhanced documentation to mention that.

@iloveeclipse
Copy link
Copy Markdown
Member

@akurtakov, do I miss something? PR description says about StackOverflow fix, but code change shows only javadoc comment changed?

Copy link
Copy Markdown
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the installer was broken from stack overflow in org.eclipse.emf.edit.ui.provider.ImageWrapperImageDescriptor

@akurtakov
Copy link
Copy Markdown
Member Author

@akurtakov, do I miss something? PR description says about StackOverflow fix, but code change shows only javadoc comment changed?

I forgot to push that part initially. Should be good now.

@merks
Copy link
Copy Markdown
Contributor

merks commented Sep 23, 2025

FYI, this was the stack overflow

image

@iloveeclipse
Copy link
Copy Markdown
Member

iloveeclipse commented Sep 23, 2025

I forgot to push that part initially. Should be good now.

Good, but if that was automated cleanup that redirects from deprecated to non deprecated methods, wouldn't that hit again on next cleanup round?

@merks
Copy link
Copy Markdown
Contributor

merks commented Sep 23, 2025

I forgot to push that part initially. Should be good now.

Good, but if that was automated cleanup that redirects from deprecated to non deprecated methods, wouldn't that hit again on next cleanup round?

Yes, I had the same question. But apparently the cleanup considers the Javadoc to determine the replacement call and @akurtakov tested that the changes to the Javadoc prevents the save action (automated by the cleanup bot) from making that change again. We should keep and eye on this nevertheless!

Fixes getImageData(100) calling itself endlessly (introduced via
eclipse-platform@c2fe144
) by reintroducing the bigger getImageData(100) and getImageData()
endlessly (introduced in 2017 via
eclipse-platform@e6d12ca
) that wasn't experienced thanks to the *happy accident* that it was
asbtract method and all implementations before that had their own and
every implementor since then most likely figured the endless loop while
developing.
Enhanced documentation to mention that.
@akurtakov
Copy link
Copy Markdown
Member Author

Save action runs that changes deprecated calls relies on deprecated javadoc tag content (https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/210f65acc02641c0e26a081aa88ca119c4c2b2ca/org.eclipse.jdt.core.manipulation/common/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessorUtil.java#L591 ) which explicitly kicks in only for "use" and "replace by" thus I changed the javadoc to "replace with" for the sake of differing.

@iloveeclipse
Copy link
Copy Markdown
Member

thus I changed the javadoc to "replace with" for the sake of differing.

Hmm...

Next time someone will change javadoc & face same issue again.

@jjohnstn : would it be possible to exclude automated replacement of deprecated methods via some kind of "do_not_fix_me" tag on the code that uses deprecated method?

@github-actions
Copy link
Copy Markdown
Contributor

Test Results

 2 904 files  ±0   2 904 suites  ±0   1h 59m 40s ⏱️ - 4m 9s
 8 019 tests ±0   7 774 ✅ +1  245 💤 ±0  0 ❌  - 1 
23 591 runs  ±0  22 809 ✅ +1  782 💤 ±0  0 ❌  - 1 

Results for commit 40924de. ± Comparison against base commit 4f687d6.

@laeubi
Copy link
Copy Markdown
Contributor

laeubi commented Sep 23, 2025

@akurtakov javaodc of the class says:

Using this abstract class involves defining a concrete subclass
and re-implementing the {@link #getImageData(int)} method.
Legacy subclasses that are not HiDPI-aware used to override the
deprecated {@link #getImageData()} method. Subclasses must
re-implement exactly one of the {@code getImageData} methods

and they should not re-implement both.

So does this javadoc needs fixing?

@akurtakov
Copy link
Copy Markdown
Member Author

IMO one could drop the note about reimplement exactly one but I haven't given it enough thought. I'm merging this one to prevent the regression and starting new I-build. Documentation enhancement can happen in follow up PRs.

@akurtakov akurtakov merged commit 422bd8b into eclipse-platform:master Sep 23, 2025
18 checks passed
@akurtakov
Copy link
Copy Markdown
Member Author

@merks
Copy link
Copy Markdown
Contributor

merks commented Sep 23, 2025

Thank you. ❣️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants